Skip to content

[ENH] Add feature to the rust log service to proxy the go log service. #4559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 21, 2025
Merged

[ENH] Add feature to the rust log service to proxy the go log service. #4559

merged 1 commit into from
May 21, 2025

Conversation

rescrv
Copy link
Contributor

@rescrv rescrv commented May 15, 2025

Description of changes

Following the rollout ADR, this will encode table to go
classic->transitioning->transitioned.

inactive active
open classic BUG
sealed transitioning transitioned

Test plan

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

N/A

@github-actions GitHub Actions
Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@rescrv rescrv force-pushed the rescrv/rust-proxies-go branch from 613c61f to a5bd3fa Compare May 16, 2025 17:24
@rescrv rescrv changed the base branch from rescrv/sealing-log-service to rescrv/wal3-bootstrap-impl May 16, 2025 17:25
@rescrv rescrv force-pushed the rescrv/wal3-bootstrap-impl branch from 51295b2 to cad8641 Compare May 16, 2025 19:44
@rescrv rescrv force-pushed the rescrv/rust-proxies-go branch 2 times, most recently from bb3b8b7 to c9383bd Compare May 16, 2025 22:45
@rescrv rescrv changed the base branch from rescrv/wal3-bootstrap-impl to rescrv/scout-logs-both-bounds May 19, 2025 15:16
@rescrv rescrv force-pushed the rescrv/rust-proxies-go branch from 5bc99c6 to 324da65 Compare May 19, 2025 16:04
@rescrv rescrv marked this pull request as ready for review May 19, 2025 16:06
@propel-code-bot propel-code-bot
Copy link
Contributor

propel-code-bot bot commented May 19, 2025 •
edited
Loading

Rust Log Service Proxy to Go Log Service Implementation

This PR introduces a significant integration between the Rust log service and the legacy Go log service, enabling the Rust service to proxy requests to the Go log as part of a hybrid rollout/migration strategy (per rollout ADR). The Rust log service can now forward RPCs such as PushLogs, ScoutLogs, PullLogs, UpdateCollectionLogOffset, and ForkLogs to the Go service if a proxy is configured and the local log state is uninitialized, thereby supporting collection state transitions ('classic', 'transitioning', 'transitioned'). Major updates span code logic, configuration files, and supporting client code for gRPC connectivity.

Key Changes:
• Addition of proxying capability in the Rust log service to forward gRPC requests to the Go log service using the new proxy_to configuration.
• Conditionally forwards ('proxies') several log-related RPCs (push, pull, scout, fork, update offset) when the local Rust log is uninitialized or still relying on the Go backend.
• Enhancements to configuration (tilt_config.yaml, LogServerConfig) with proxy_to options for specifying Go log service endpoints.
• Refactors in error handling to support proxy fallback paths and distinguish between uninitialized logs and genuine errors.
• Extension of Rust log gRPC client code to allow creating direct clients from configuration, used by the proxy mechanism.
• Cargo.lock update for dependency/version management.

Affected Areas:
• rust/log-service/src/lib.rs (core Rust log server logic and proxying paths)
• rust/log/src/grpc_log.rs (grpc client extensions, helper for direct client instantiation)
• rust/worker/tilt_config.yaml (configuration: new proxy_to support for log service)
• Cargo.lock (dependency update for chroma-cli version bump)

Potential Impact:

Functionality: Critical for staged migration of collections from Go to Rust log service, with seamless request forwarding and log transfer when needed; changes affect core RPC pathing during runtime and impact all log service consumers.

Performance: Adds some overhead for proxied requests; performance may be impacted when proxying to the Go log service, especially during migrations or if proxy_to is always active; local operations remain unaffected after migration.

Security: No new direct security exposure but introduces dependency on Go log endpoint being secure and properly configured; improper configuration could result in failed log operations.

Scalability: Increases system resilience during the migration window by allowing split traffic; system remains as scalable as underlying Go/Rust services.

Review Focus:
• Correctness and atomicity of proxy logic (especially effectuate_log_transfer and RPC fallback/forwarding).
• Behavior under partial or incorrect proxy configuration (proxy_to absent or mis-set in YAML/config).
• Error and edge-case handling (e.g., UninitializedLog, transient network or service errors).
• Backward compatibility for collections that are already migrated vs. those pending migration.
• Configuration documentation and roll-forward/rollback safety.

Testing Needed

• Test all log-related RPCs with and without a proxy_to configuration.
• Test collection creation, migration, and access for classic, transitioning, and transitioned tables.
• Validate proper error propagation and fallback logic when Go log proxy is unavailable or misconfigured.
• Benchmark performance before and after proxying to assess any regressions.
• Review log outputs for correct proxy initialization, fallback, and error cases.

Code Quality Assessment

rust/log-service/src/lib.rs: Well-structured, with clear separation of proxy and local paths. Correct use of async and error propagation. Some duplicative logic noted as a temporary measure in comments.

rust/log/src/grpc_log.rs: Code is clear and documented; the client creation logic is duplicated but noted as a transient hack for this phase of rollout. No major Rust best-practice violations.

rust/worker/tilt_config.yaml: Appropriate extension, config structure remains comprehensible.

Cargo.lock: Version as expected with chroma-cli.

Best Practices

Observability:
• Traces and logs for proxying/fallback paths

Migration:
• Graceful state transition with minimal client disruption

Error Handling:
• Propagation of meaningful error codes, typed errors for state transitions

Configuration:
• Backward-compatible config changes with defaults

Potential Issues

• Proxy remains optional (Option<...>)-consider hard-fail vs. silent neutral response if config is missing when proxying is expected.
• If proxy_to misconfigured, log service becomes partially/non-operational for not-yet-migrated collections.
• Some duplicative logic (noted in comments as temporary) may be a maintenance issue if left long-term.
• Performance hit if proxying is used for every request-should be monitored.
• YAML config could lead to operator confusion regarding roll-forward/rollback toggles.

This summary was automatically generated by @propel-code-bot

@rescrv rescrv requested review from codetheweb and sanketkedia May 19, 2025 16:07
@rescrv rescrv force-pushed the rescrv/rust-proxies-go branch from 324da65 to 4c6289d Compare May 19, 2025 16:49
codetheweb
codetheweb approved these changes May 19, 2025
View reviewed changes
rust/log-service/src/lib.rs
) -> Result<Response<PushLogsResponse>, Status> {
let request = request.into_inner();
if let Some(proxy) = self.proxy.as_ref() {
let resp = proxy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for my understanding, the go service will continue to be called for every PushLogs invocation, even on migrated collections?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not on migrated collections. We only get to forward-push-logs if the log isn't initialized.

rust/log-service/src/lib.rs
@@ -630,11 +632,179 @@ pub struct LogServer {
storage: Arc<Storage>,
open_logs: Arc<StateHashTable<LogKey, LogStub>>,
dirty_log: Arc<LogWriter>,
#[allow(clippy::type_complexity)]
proxy: Option<LogServiceClient<chroma_tracing::GrpcTraceService<tonic::transport::Channel>>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why is proxy an optional if pretty much every method needs it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the config could be lacking it, so we neutralize rather than hard fail.

@rescrv rescrv force-pushed the rescrv/scout-logs-both-bounds branch from 7a9ea53 to dc65e79 Compare May 19, 2025 17:49
@rescrv rescrv force-pushed the rescrv/rust-proxies-go branch from 4c6289d to 55f4aae Compare May 19, 2025 17:50
@rescrv rescrv changed the base branch from rescrv/scout-logs-both-bounds to rescrv/cops-idl May 19, 2025 17:51
@rescrv rescrv force-pushed the rescrv/cops-idl branch from 9eb5315 to c52b4e4 Compare May 19, 2025 18:26
@rescrv rescrv force-pushed the rescrv/rust-proxies-go branch from 55f4aae to 93e7d3e Compare May 19, 2025 18:27
@rescrv rescrv force-pushed the rescrv/cops-idl branch from c52b4e4 to 3da2abd Compare May 19, 2025 19:46
@rescrv rescrv force-pushed the rescrv/rust-proxies-go branch from 93e7d3e to f1c8390 Compare May 19, 2025 19:47
@rescrv rescrv force-pushed the rescrv/cops-idl branch from 3da2abd to abf5b1e Compare May 20, 2025 23:17
@rescrv rescrv force-pushed the rescrv/rust-proxies-go branch from f1c8390 to e4a1e47 Compare May 20, 2025 23:21
@rescrv rescrv force-pushed the rescrv/cops-idl branch from abf5b1e to 12ca8e1 Compare May 21, 2025 01:18
Base automatically changed from rescrv/cops-idl to main May 21, 2025 02:14
@rescrv
[ENH] Add feature to the rust log service to proxy the go log service.
3682d5c 
Following the rollout ADR, this will encode table to go
classic->transitioning->transitioned.

|            | **inactive**  | **active**   |
|------------|---------------|--------------|
| **open**   | classic       | BUG          |
| **sealed** | transitioning | transitioned |
@rescrv rescrv force-pushed the rescrv/rust-proxies-go branch from e4a1e47 to 3682d5c Compare May 21, 2025 16:59
@rescrv rescrv merged commit 627faa7 into main May 21, 2025
70 checks passed
@rescrv rescrv deleted the rescrv/rust-proxies-go branch May 21, 2025 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codetheweb codetheweb codetheweb approved these changes

@sanketkedia sanketkedia Awaiting requested review from sanketkedia

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rescrv @codetheweb